-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Block Deprecations: Provide extra data for isEligible check #48815
Conversation
@youknowriad would you mind weighing in on this change or suggesting a better alternative? |
Size Change: +1.83 kB (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
Flaky tests detected in 2532a46. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4422539506
|
I think we've found in the past that changing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I don't like the current isEligible
API, and I'd say that our deprecation behavior/algorithm is not perfect at all but this is going to be a huge effort to improve. Now passing the block like done in this PR feels a bit random. Maybe a new API is a bit better if this proves to be really necessary.
Thanks @youknowriad 👍
Could this PR around the layout At first glance, changing the default layout for the Group block definitely had wider ranging impacts. The media and text block default alignment seemed reduced in scope but I'm happy to be warned of unconsidered pitfalls though.
100% agreed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping @youknowriad. Sharing my approval here optimistically, to note my approval of the idea, not to signal that you ought to merge this without the review and feedback from others 😉
Now passing the block like done in this PR feels a bit random. Maybe a new API is a bit better if this proves to be really necessary.
Given that deprecations already have trouble with the designed API I'm more inclined to let the extra parameters in. To that end I think rawBlock
is more appropriate than block
because of the implications of block
at this point.
We can call rawBlock
the blockNode
(hat-tip to @mcsf who I think coined that term) or the "stage-1 parse of the block," which is what we get when parsing the serialized HTML.
The block
object here, or the "stage-2 parse," is the result of feeding that block node into the code for the given block type.
The distinction might be important here because we're in a situation where we know something didn't load as expected and that block
is likely invalid (according to what the editor calls "invalid"). It can be misleading because it may have lost correspondence with the serialized HTML. For example, the list block updated to use inner blocks, so old list blocks will have the innerHTML
in that block node with the <ul>
(or <ol>
) and <li>
tags, but will render an empty list into block
, because there are no inner blocks.
A deprecation would need to restore the original content that in a fraction of the cases will be wiped out before running the deprecation code.
our deprecation behavior/algorithm is not perfect at all but this is going to be a huge effort to improve
this is another reason that being loose here feels favorable: an overhaul of the block updating and validation-handling code might wipe out a number of these fixes through very different means and I suspect there's little likelihood we can anticipate now how those will appear later.
Thanks for weighing in @dmsnell you make some great points 👍 It still appears to me that there could be some benefit in proceeding with the addition of the extra args to I don't think there is a huge rush on this so it can wait for sufficient reviews for everyone to be confident in the change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for reworking the docs.
I left a few suggestions for improving consistency, as there were a few different terms being used (like 'serialized HTML' vs 'saved markup') that mean the same thing, but I leave these up to you.
Co-authored-by: Daniel Richards <[email protected]>
Thanks. The suggestions look good. I've committed those and will merge once the e2es pass. |
Related:
none
#48404What?
Provides additional data from which deprecations can check if they are eligible.
Why?
The information currently passed to the
isEligible
check is insufficient in some cases to determine if a deprecation should in fact run.One use case requiring additional information can be found in:
none
#48404In the above example, when the default
align
attribute has its default switched tonone
, existing blocks using the defaultwide
align are not determined to be invalid. This is due to the custom CSS class name support picking up the deprecatedalignwide
class from the markup and adding it to theclassName
attribute.As the deprecated block is still determined to be valid, the deprecation's
isEligible
callback must be able to return true in this scenario. The catch is presently theisEligible
function is passed only the raw parsed attributes and inner blocks. The raw parsed attributes do not yet have any amendments made to theclassName
attribute by the custom class names support.How?
isEligible
function. That being and object now including the current block e.g.{ block }
.isEligible
function definitions.Alternative Approaches
Pass only
rawBlock
and currentblock
as extra paramsThis would avoid the creation of a new object and potential minimise any performance hit. It would make for a rather awkward signature and API.
isEligibleV2
This approach would allow deprecations to define their eligibility check via a new function signature.
e.g.
isEligibleV2( rawBlock, currentBlock )
.This would still provide the existing data via
rawBlock.attributes
andcurrentBlock.innerBlocks
but also allow checks to be made against the markup or finalized block attributes.Some downsides are the multiple function signatures, the potential need to deprecate one eventually, potential confusion etc. The upside is primarily the clean function signature and only passing the extra data when a deprecation desires it.
Adding a disallow list to Custom CSS Classname support
This is a more heavy-handed and hacky approach with a greater potential performance hit.
Blocks could define a disallow list for custom class names. That list can be checked during the determination of custom class names from the block's markup. A CSS class is only added to the
className
attribute if it wasn't in the disallow list.Using the media and text example,
alignwide
could be disallowed, which would then cause unmigrated blocks to be invalidated triggering the necessary deprecation without the need for theisEligible
check.Testing Instructions
npm run test:unit test/integration/full-content/full-content.test.js
none
#48404 which is now based on this PR